Preserve NextRetryDelay across unrelated activity option updates#10920
Conversation
When a running activity has a deferred reset with keep_paused=true, unpause should clear only ResetKeepPaused while preserving the pending reset.
Validate ScheduleToClose timer tasks by exact stamp match so legacy zero-stamp tasks are only valid while the activity stamp is still zero.
|
|
||
| // start_delay updates are only valid while the activity is still in its delay window. | ||
| _, hasStartDelayInMask := updateFields["startDelay"] | ||
| if !frontendReq.GetRestoreOriginal() && hasStartDelayInMask { |
There was a problem hiding this comment.
RestoreOriginal should not be combined with any update mask and should be rejected in validation which means we should be able to use if hasStartDelayInMask here
There was a problem hiding this comment.
Actually looks like we already do that (chasm/lib/activity/validator.go:472-474) so we can simplify to just if hasStartDelayInMask
| }, nil | ||
| } | ||
|
|
||
| func (a *Activity) shouldRecalculateCurrentRetryInterval( |
There was a problem hiding this comment.
doc comment for the logic here would be helpful
There was a problem hiding this comment.
as I understand it we recalc if all of the following are true:
- activity is not scheduled or paused
- the update included some change to retryPolicy, either the whole policy or at least one subfield
- AND there isn't a worker provided NextRetryDelay
Is that correct?
There was a problem hiding this comment.
Added docs.
Mostly, with corrections:
- For state, it's the opposit. We recalc only when the activity is in SCHEDULED or PAUSED (i.e. waiting to retry, so there's a pending backoff to adjust). A running attempt has no pending retry interval to recalculate.
- the "update touched retryPolicy" check only applies to normal mask updates. On RestoreOriginal we skip that check entirely, since restore swaps in the whole original policy and should recalc regardless of the mask.
|
|
||
| if !restoreOriginal { | ||
| hasRetryPolicyInMask := false | ||
| for field := range updateFields { |
There was a problem hiding this comment.
| for field := range updateFields { | |
| if _, ok := updateFields["retryPolicy"]; ok { | |
| hasRetryPolicyInMask = true | |
| } else { | |
| for field := range updateFields { | |
| if strings.HasPrefix(field, "retryPolicy.") { | |
| hasRetryPolicyInMask = true | |
| break | |
| } | |
| } | |
| } |
There was a problem hiding this comment.
If you're not strongly opinonated on this, I'd like to keep what I have as I feel it's shorter and more readable
| }) | ||
| require.NoError(t, err) | ||
|
|
||
| pollCtx, pollCancel := context.WithTimeout(ctx, 8*time.Second) |
There was a problem hiding this comment.
I'm confused by this section, if the retry delay is 10minutes originally why would this activity be available in the polling? Shouldn't the polling timeout with context deadline exceeded?
There was a problem hiding this comment.
The 10-minute InitialInterval is overridden here: the worker sets NextRetryDelay in RespondActivityTaskFailed, so the actual backoff is 2s, not 10m. Added a comment here to reduce confusion.
There was a problem hiding this comment.
Ahhh missed that in the failed, thanks for clarifying.
| // CurrentRetryInterval stores either policy-derived backoff or worker-provided NextRetryDelay overrides. | ||
| // Only recalculate intervals that match the old policy value; different values are treated as explicit | ||
| // worker overrides. | ||
| return currentRetryInterval.AsDuration() == policyRetryIntervalBeforeUpdate |
There was a problem hiding this comment.
Can you send consecutive updates and recompute the retry interval on the second one? The currentRetryInterval here is just the last computed one I think, not the original retry interval from before a potential first retry interval created.
There was a problem hiding this comment.
Good point out. Consecutive updates do recompute correctly. policyRetryIntervalBeforeUpdate is recomputed each call from the current a.RetryPolicy, and each update persists both the new policy and the recalculated interval, so they stay in sync. The second update compares against update 1's value (not the original) and recalculates as expected. I added a test to cover this case
) ## What changed? Fixed standalone activity update-options retry handling so unrelated option updates no longer recalculate an already-scheduled worker-provided NextRetryDelay. ## Why? CurrentRetryInterval stores both retry-policy backoff and worker-provided NextRetryDelay. The old update path recalculated this field for any scheduled/paused activity update, so an unrelated update could overwrite a worker override and delay the next retry incorrectly. ## How did you test it? - [X] built - [X] run locally and tested manually - [ ] covered by existing tests - [X] added new unit test(s) - [X] added new functional test(s)
## What changed? Fixed standalone activity option updates so a pending retry is not delayed when the worker-provided `NextRetryDelay` happens to equal the old retry-policy interval. When recalculating `CurrentRetryInterval` after `UpdateActivityExecutionOptions`, the activity now only applies the newly computed policy interval if it shortens the already scheduled retry delay. This preserves the existing retry schedule when a retry-policy update would otherwise move the next attempt later. Added a regression test for the case where: - attempt 1 fails with `NextRetryDelay=2m` - the current retry policy also had `InitialInterval=2m` - the retry policy is updated to `InitialInterval=10m` - the pending retry must remain scheduled at 2m, not move to 10m ## Why? #10920 preserved worker-provided `NextRetryDelay` when it differed from the policy-derived interval, but missed the edge case where the worker delay and old policy interval were equal. In that case, the update path treated the pending retry as policy-derived and recalculated it from the new policy, incorrectly extending the already scheduled retry from 2m to 10m. A retry-policy update should not delay an existing worker-controlled retry. ## How did you test it? - [ ] built - [ ] run locally and tested manually - [x] covered by existing tests - [ ] added new unit test(s) - [x] added new functional test(s) ## Potential risks Minimal, this is still part of our testing push and not released to production.
2 participants
What changed?
Fixed standalone activity update-options retry handling so unrelated option updates no longer recalculate an already-scheduled worker-provided NextRetryDelay.
Why?
CurrentRetryInterval stores both retry-policy backoff and worker-provided NextRetryDelay. The old update path recalculated this field for any scheduled/paused activity update, so an unrelated update could overwrite a worker override and delay the next retry incorrectly.
How did you test it?